Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that the annotationLayer has the correct dimensions (PR 15036 follow-up) #15069

Merged

Conversation

Snuffleupagus
Copy link
Collaborator

Note how the "page"-div, "canvasWrapper"-div, and textLayer-div all have integer dimensions (rounded down) rather than using the "raw" viewport-dimensions.
Hence it seems reasonable that the same should apply to the "annotationLayer"-div, now that it's explicit dimensions set.

…follow-up)

Note how the "page"-div, "canvasWrapper"-div, and `textLayer`-div all have *integer* dimensions (rounded down) rather than using the "raw" viewport-dimensions.
Hence it seems reasonable that the same should apply to the "annotationLayer"-div, now that it's explicit dimensions set.
@Snuffleupagus
Copy link
Collaborator Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/67f06d77563854c/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/c2d548d1c8800c1/output.txt

@calixteman
Copy link
Contributor

Do we need to have the "exact" width/height in the viewport ? If not, we could just store the floored values, wdyt ?

@Snuffleupagus
Copy link
Collaborator Author

Do we need to have the "exact" width/height in the viewport ?

I'd be somewhat nervous about making that sort of change, since the current behaviour has existing since "forever" and changing it would require going through and (often manually) testing lots of code.
To me, it feels like there's probably better things to spend time/effort on :-)

If not, we could just store the floored values, wdyt ?

As a start, we could perhaps also provide the floored dimensions directly on PageViewport-instances to save us from re-computing it multiple times.

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/67f06d77563854c/output.txt

Total script time: 26.22 mins

  • Font tests: Passed
  • Unit tests: FAILED
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 93
  different first/second rendering: 1

Image differences available at: http://54.241.84.105:8877/67f06d77563854c/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/c2d548d1c8800c1/output.txt

Total script time: 28.84 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 37

Image differences available at: http://54.193.163.58:8877/c2d548d1c8800c1/reftest-analyzer.html#web=eq.log

@calixteman
Copy link
Contributor

/botio browsertest

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_browsertest from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/467e85150c03528/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_browsertest from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/8bcdb720d99955b/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/8bcdb720d99955b/output.txt

Total script time: 21.48 mins

  • Regression tests: FAILED
  different ref/snapshot: 32

Image differences available at: http://54.193.163.58:8877/8bcdb720d99955b/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/467e85150c03528/output.txt

Total script time: 22.08 mins

  • Regression tests: FAILED
  different ref/snapshot: 80

Image differences available at: http://54.241.84.105:8877/467e85150c03528/reftest-analyzer.html#web=eq.log

@calixteman
Copy link
Contributor

In debugging some code I noticed:
image
and the dimensions are correctly floored to 1019 and 1319 and I just wondered if we should round instead.

@Snuffleupagus
Copy link
Collaborator Author

and the dimensions are correctly floored to 1019 and 1319 and I just wondered if we should round instead.

Given that we're using Math.floor in all other cases, we really want to be consistent here as far as I'm concerned. As mentioned before, I'm not too keen on going through every call-site and trying to ensure that changing this everywhere won't cause problems (since part of this code is very old).

Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

@Snuffleupagus Snuffleupagus merged commit db6f675 into mozilla:master Jun 21, 2022
@Snuffleupagus
Copy link
Collaborator Author

/botio makeref

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_makeref from @Snuffleupagus received. Current queue size: 1

Live output at: http://54.193.163.58:8877/1cca1343973603f/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_makeref from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/901be1a97d2d83c/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/901be1a97d2d83c/output.txt

Total script time: 22.75 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://54.193.163.58:8877/1cca1343973603f/output.txt

Total script time: 22.06 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants